[Verifier] Require that dbg.declare variable is a ptr #134355
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As far as I understand, the first operand of dbg_declare should be a pointer (inside a metadata wrapper). However, using a non-pointer is currently not rejected, and we have some tests that use non-pointer types. As far as I can tell, these tests either meant to use dbg_value or are just incorrect hand-crafted tests. Ran into this while trying to fix llvm#134008.
llvmbot
added
backend:AArch64
backend:X86
debuginfo
coroutines
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-backend-x86 Author: Nikita Popov (nikic) ChangesAs far as I understand, the first operand of dbg_declare should be a pointer (inside a metadata wrapper). However, using a non-pointer is currently not rejected, and we have some tests that use non-pointer types. As far as I can tell, these tests either meant to use dbg_value or are just incorrect hand-crafted tests. Ran into this while trying to Full diff: https://github.com/llvm/llvm-project/pull/134355.diff 8 Files Affected:
|
OCHyams
reviewed
Apr 4, 2025
| %rand = tail call i64 @lrand48() #3 | ||
| tail call void @llvm.dbg.declare(metadata i64 %rand, metadata !6, metadata !7), !dbg !8 | ||
| tail call void @llvm.dbg.declare(metadata ptr %ptr, metadata !6, metadata !7), !dbg !8 |
There was a problem hiding this comment.
I think this slightly changes the tested behaviour, because the new dbg.declare will get put in the MF side-table:
https://godbolt.org/z/bh6oEM55G
The old dbg.declare causes a DBG_INSTR_REF, whereas the new on gets stuffed into stack:. I think this may be the case in a couple of other tests too. If you make %ptr an argument instead of a local alloca it looks like a DBG_VALUE is generated, which is closer to the output of the original test.
Since the test isn't checking for the debug instruction in the output, maybe it doesn't matter, but I'd personally err on the side of minimising the change in output. YMMV.
SLTozer
reviewed
Apr 4, 2025
There was a problem hiding this comment.
Principle SGTM, probably calls for a documentation update: this could be extremely minor just to stress the "new" requirement, updating the text for #dbg_declare in SourceLevelDebugging.rst:'
- The first argument is an SSA value corresponding to a variable address, and is
+ The first argument is an SSA `ptr` value corresponding to a variable address, and is
llvm/lib/IR/Verifier.cpp
Outdated
| if (DVR.isDbgDeclare()) | ||
| CheckDI(VAM->getValue()->getType()->isPointerTy(), | ||
| "location of #dbg_declare must be a pointer", &DVR, MD); | ||
| } else if (auto *AL = dyn_cast<DIArgList>(MD)) |
There was a problem hiding this comment.
Nit, since the if now uses braces the "else if" should as well.
| @@ -55,8 +55,9 @@ end: ; preds = %body | |||
|
|
|||
| define i64 @simulateWithDbgDeclare(<2 x i32> %a) local_unnamed_addr { | |||
| entry: | |||
| %ptr = alloca i32 | |||
There was a problem hiding this comment.
Slightly awkward case here in that this might degrade the test quality... the intent here seems to be to test that the debug declare does not cause any changes to the code in the CHECK set below, so making it no longer use %rand as an argument might interfere with that.
But otoh the test is non-specific; there's no description of how the debug declare might be expected to affect codegen, so I don't think I have a specific suggested change - you could add a store i64 %rand, ptr %ptr, or expand the check to cover the generated code for %ptr, but I also don't think it's a blocking issue if it stays as-is.
There was a problem hiding this comment.
Hm, adding a store like that would affect the existing CHECK lines, so I'm not sure that's better...
There was a problem hiding this comment.
Yeah, I'm not convinced there are any solid answers - the test is to check that "nothing bad happens" because of the presence of a dbg.declare, rather than checking for a specific defect, so it's not clear what properties would need to be maintained to keep the test valid. If there aren't any obvious solutions, I'm fine with the latest revision.
SLTozer
approved these changes
Apr 4, 2025
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
Apr 7, 2025
When salvaging debug info for #dbg_declare, do not perform salvage that changes the type of the value, i.e. bail out on cast instructions. This ensures that we don't run afoul of the verifier check in llvm#134355.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As far as I understand, the first operand of dbg_declare should be a pointer (inside a metadata wrapper). However, using a non-pointer is currently not rejected, and we have some tests that use non-pointer types. As far as I can tell, these tests either meant to use dbg_value or are just incorrect hand-crafted tests.
Ran into this while trying to
fix#134008.